frontend/src/components/ui/button.tsx (1)

8-8: Good UX improvement with cursor-pointer, but disabled:cursor-not-allowed is redundant.

The cursor-pointer addition improves button UX by indicating clickability. However, disabled:cursor-not-allowed has no effect because disabled:pointer-events-none is already set—when pointer events are disabled, the browser won't display custom cursor styles.

If you want to keep the class string cleaner, you can remove the redundant class:

-  "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md text-sm font-medium transition-all cursor-pointer disabled:pointer-events-none disabled:opacity-50 disabled:cursor-not-allowed [&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0 outline-none focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:ring-[3px] aria-invalid:ring-destructive/20 dark:aria-invalid:ring-destructive/40 aria-invalid:border-destructive",
+  "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md text-sm font-medium transition-all cursor-pointer disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0 outline-none focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:ring-[3px] aria-invalid:ring-destructive/20 dark:aria-invalid:ring-destructive/40 aria-invalid:border-destructive",

backend/app/schemas/videos.py (1)

5-13: Consider adding field validators for VideoMetadata.

The VideoMetadata model lacks validation constraints. Consider adding validators to ensure data integrity (e.g., positive values for width, height, duration, and file_size).

Example validation:

from pydantic import BaseModel, Field

class VideoMetadata(BaseModel):
    name: str
    date_created: Optional[str] = None
    width: int = Field(gt=0)
    height: int = Field(gt=0)
    duration: float = Field(gt=0)
    file_location: str
    file_size: int = Field(ge=0)
    item_type: str

backend/app/config/settings.py (1)

27-29: Consider using absolute paths for video storage.

The relative paths ./videos and ./videos/thumbnails depend on the current working directory. While this is consistent with the existing IMAGES_PATH pattern, consider using pathlib.Path with absolute paths or environment variables for more robust path handling across different execution contexts.

frontend/src/types/Media.ts (1)

40-48: Consider nullable thumbnailPath for backend consistency.

The thumbnailPath field is typed as string (line 43), but the backend VideoData.thumbnailPath is Optional[str]. Consider using thumbnailPath: string | null to handle cases where thumbnails might not be available.

 export interface Video {
   id: string;
   path: string;
-  thumbnailPath: string;
+  thumbnailPath: string | null;
   folder_id?: string;
   metadata?: VideoMetadata;
   title?: string;
   tags?: string[];
 }

backend/app/routes/folders.py (2)

71-73: Consider checking video processing return value.

While video_util_process_folder_videos returns a boolean indicating success/failure, the return value is not checked. If video processing fails, the function continues silently. Consider logging the result or adding explicit error handling for better observability.

 # Process images and videos in all folders
 image_util_process_folder_images(folder_data)
-video_util_process_folder_videos(folder_data)
+video_success = video_util_process_folder_videos(folder_data)
+if not video_success:
+    logger.warning(f"Video processing encountered errors for folder {folder_path}")

---

119-121: Consider checking video processing return value.

Similar to the add folder sequence, the return value of video_util_process_folder_videos is not checked here. Consider adding result validation for consistency and better error visibility.

 # Process images and videos in all folders
 image_util_process_folder_images(folder_data)
-video_util_process_folder_videos(folder_data)
+video_success = video_util_process_folder_videos(folder_data)
+if not video_success:
+    logger.warning(f"Video processing encountered errors during sync for folder {folder_path}")
 image_util_process_untagged_images()

frontend/src/components/VideoPlayer/PlyrPlayer.tsx (1)

102-102: Consider supporting multiple video formats.

The video source is hardcoded to type="video/mp4", which may not support other formats like WebM or OGV.

If you need to support multiple formats, consider:

-        <source src={src} type="video/mp4" />
+        <source src={src} />

Or detect the format from the file extension and set the appropriate MIME type.

backend/app/routes/videos.py (1)

24-33: Consider extracting duplicate VideoData mapping logic.

The same VideoData construction pattern appears in all four endpoints. Extracting this into a helper function would improve maintainability.

Example:

def _map_videos_to_response(videos: List[dict]) -> List[VideoData]:
    """Helper to map database video dicts to VideoData schema."""
    return [
        VideoData(
            id=v["id"],
            path=v["path"],
            folder_id=v.get("folder_id"),
            thumbnailPath=v["thumbnailPath"],
            metadata=image_util_parse_metadata(v.get("metadata")),
        )
        for v in videos
    ]

# Then use in endpoints:
data = _map_videos_to_response(videos)

Also applies to: 57-66, 100-109, 133-142

backend/app/utils/videos.py (3)

55-81: Consider refactoring the import and directory check.

Two minor inefficiencies:

1. Import inside function (line 59): The local import of db_get_video_by_path suggests a circular dependency issue. Consider restructuring imports or using TYPE_CHECKING to resolve this more cleanly.

2. Redundant directory check (line 61): video_util_ensure_dirs() is called for every file registration. While harmless (due to exist_ok=True), it's unnecessary overhead. Consider calling it once during application startup or at the beginning of batch operations like video_util_process_folder_videos.

For the import issue, consider moving it to the top with TYPE_CHECKING:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from app.database.videos import db_get_video_by_path

Or restructure the module dependencies to eliminate the circular import.

---

90-108: Add consistent error handling for recursive mode.

The non-recursive mode catches OSError (line 106), but the recursive mode using os.walk (lines 94-99) doesn't. The os.walk function can raise OSError for permission issues or other filesystem errors when traversing subdirectories, leading to inconsistent behavior.

Apply this diff to add consistent error handling:

 def video_util_get_videos_from_folder(
     folder_path: str, recursive: bool = True
 ) -> List[str]:
     videos: List[str] = []
     if recursive:
-        for root, _, files in os.walk(folder_path):
-            for f in files:
-                p = os.path.join(root, f)
-                if video_util_is_valid_video(p):
-                    videos.append(p)
+        try:
+            for root, _, files in os.walk(folder_path):
+                for f in files:
+                    p = os.path.join(root, f)
+                    if video_util_is_valid_video(p):
+                        videos.append(p)
+        except OSError:
+            pass
     else:
         try:
             for f in os.listdir(folder_path):
                 p = os.path.join(folder_path, f)
                 if os.path.isfile(p) and video_util_is_valid_video(p):
                     videos.append(p)
         except OSError:
             pass
     return videos

---

114-126: Add logging for top-level exceptions.

The top-level exception handler (lines 125-126) silently returns False without logging the error. This makes it difficult to diagnose why folder processing failed, especially since the function processes multiple folders and could fail for various reasons (filesystem issues, database errors, etc.).

Apply this diff to add error logging:

 def video_util_process_folder_videos(folder_data: List[essential_tuple]) -> bool:
     try:
         video_util_ensure_dirs()
         for path, folder_id, recursive in folder_data:
             file_list = video_util_get_videos_from_folder(path, recursive)
             for fp in file_list:
                 try:
                     video_util_register_file(fp, folder_id)
                 except Exception:
                     continue
         return True
-    except Exception:
+    except Exception as e:
+        logger.error(f"Failed to process folder videos: {e}")
         return False

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

backend/app/routes/videos.py (4)

9-9: Consider adding tags to the router for better API documentation.

Adding tags helps organize endpoints in the automatically generated OpenAPI/Swagger documentation.

Apply this diff:

-router = APIRouter()
+router = APIRouter(tags=["videos"])

---

12-39: Consider pagination for scalability.

The endpoint returns all videos without pagination, which could cause performance issues and large response sizes with extensive video libraries.

Consider adding pagination parameters:

@router.get(
    "/",
    response_model=GetAllVideosResponse,
    responses={500: {"model": ErrorResponse}},
)
def get_all_videos(skip: int = 0, limit: int = 100):
    try:
        videos = db_get_all_videos()
        # Apply pagination
        paginated_videos = videos[skip:skip + limit]
        data: List[VideoData] = [
            VideoData(
                id=v["id"],
                path=v["path"],
                folder_id=v.get("folder_id"),
                thumbnailPath=v["thumbnailPath"],
                metadata=image_util_parse_metadata(v.get("metadata")),
            )
            for v in paginated_videos
        ]
        return GetAllVideosResponse(
            success=True, 
            message=f"Retrieved {len(data)} videos (showing {skip}-{skip+len(data)} of {len(videos)} total)", 
            data=data
        )
    except Exception as e:
        raise HTTPException(
            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
            detail=ErrorResponse(
                success=False, error="Internal server error", message=str(e)
            ).model_dump(),
        )

---

47-47: Remove async keyword or use asynchronous operations.

The function is declared as async but doesn't use any await expressions. This creates unnecessary overhead and inconsistency with get_all_videos, which is synchronous.

Apply this diff:

-async def scan_videos_from_folders():
+def scan_videos_from_folders():

---

80-80: Remove async keyword or use asynchronous operations.

Similar to scan_videos_from_folders, this function is declared as async but doesn't use any await expressions, creating inconsistency with get_all_videos.

Apply this diff:

-async def cleanup_deleted_videos():
+def cleanup_deleted_videos():

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

• frontend/package-lock.json: Language not supported

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

backend/main.py (2)

49-50: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard empty parent directory before makedirs

At Line 49-50, os.path.dirname(DATABASE_PATH) may be "" (e.g., filename-only DB path), which makes os.makedirs("") fail during startup.

Suggested fix

-path = os.path.dirname(DATABASE_PATH)
-os.makedirs(path, exist_ok=True)
+path = os.path.dirname(DATABASE_PATH)
+if path:
+    os.makedirs(path, exist_ok=True)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/main.py` around lines 49 - 50, When creating the DB parent directory,
guard against an empty dirname: compute path = os.path.dirname(DATABASE_PATH)
and only call os.makedirs(path, exist_ok=True) when path is non-empty (e.g., if
path). Update the block that uses DATABASE_PATH and os.path.dirname to skip
os.makedirs when dirname returns "" so startup won't call os.makedirs on an
empty string.

---

169-170: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid binding only to loopback unless intentionally local-only

At Line 169-170 (and reflected in Line 93), binding to localhost restricts access to the same host. This can break Docker/LAN access and cross-service integration if the backend is expected to be reachable externally.

Suggested fix

-        host="localhost",
+        host="0.0.0.0",

-        {"url": "http://localhost:52123", "description": "Local Development server"}
+        {"url": "http://0.0.0.0:52123", "description": "Local Development server"}

If local-only binding is intentional, document that explicitly in this file to prevent deployment regressions.

Also applies to: 93-93

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/main.py` around lines 169 - 170, The server is currently bound to
localhost (host="localhost") which prevents external/Docker/LAN access; change
the binding to allow external access by default (for example use 0.0.0.0) or
make the host configurable via an environment variable (e.g., BIND_HOST) and
default to an externally reachable address, updating the host="localhost"
occurrences in backend/main.py (the server start call) and add a short comment
documenting if loopback-only binding is intentional.